Reuse the connection attempts count logic for PQ PSK negotiation #6295
Conversation
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+Public.swift line 41 at r1 (raw file):
- Parameter nextRelay: next relay to connect to. */ public nonisolated func reconnect(to nextRelay: NextRelay, reconnectReason: ActorReconnectReason = .userInitiated) {
Implicitly "hiding" state related data (with possibly high impact) like this could end up resulting in unexpected behavior in the future.
ios/PacketTunnelCore/Actor/PacketTunnelActorCommand.swift line 22 at r1 (raw file):
/// Reconnect tunnel. case reconnect(NextRelay, reason: ActorReconnectReason = .userInitiated)
Implicitly "hiding" state related data (with possibly high impact) like this could end up resulting in unexpected behavior in the future.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/PacketTunnelCore/Actor/PacketTunnelActor+Public.swift line 41 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Implicitly "hiding" state related data (with possibly high impact) like this could end up resulting in unexpected behavior in the future.
I'm not sure what you mean by that, this is the same code, except that .userInitiated is one abstraction layer above than it used to be now.
Can you clarify what you mean by "hiding state related data" ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/PacketTunnelCore/Actor/PacketTunnelActor+Public.swift line 41 at r1 (raw file):
Previously, buggmagnet wrote…
I'm not sure what you mean by that, this is the same code, except that
.userInitiatedis one abstraction layer above than it used to be now.Can you clarify what you mean by "hiding state related data" ?
Yeah, I realised afterwards that this was already part of the current impl. What I mean is that defaulting to a state might be risky since this function can be used without realising you're supplying .userInitiated as reconnect reason. By making the param required you will always choose the correct reason, thus preventing possible bugs.
buggmagnet
force-pushed
the
pq-does-not-work-when-switching-different-port-settings-for-ios-679
branch
from
ebedeb4 to
66336ae
Compare
There was a problem hiding this comment.
Reviewable status: 8 of 12 files reviewed, 2 unresolved discussions (waiting on @rablador)
ios/PacketTunnelCore/Actor/PacketTunnelActor+Public.swift line 41 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Yeah, I realised afterwards that this was already part of the current impl. What I mean is that defaulting to a state might be risky since this function can be used without realising you're supplying
.userInitiatedas reconnect reason. By making the param required you will always choose the correct reason, thus preventing possible bugs.
Fair enough, I've made it explicit !
ios/PacketTunnelCore/Actor/PacketTunnelActorCommand.swift line 22 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Implicitly "hiding" state related data (with possibly high impact) like this could end up resulting in unexpected behavior in the future.
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
buggmagnet
deleted the
pq-does-not-work-when-switching-different-port-settings-for-ios-679
branch
|
🚨 End to end tests failed. Please check the failed workflow run. |
This PR does the following things :
ReconnectReasontoActorReconnectReasonand move it to the public interfaceconnectionLossreasonThis change is